-
-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Select root folder for timeline #515
Conversation
Signed-off-by: Corentin Mors <[email protected]>
@skjnldsv I don't know why but .jpg (which are JPEG files) are not taken in account, for some reasons, and as of now i can't find why.. |
Seems like the dav:or is not working and only the first mime type is taken in account. |
This only provides for one root folder, right? That would be a problem for me and I assume other folks too since there's a couple places photos go if you use the out-of-the-box defaults - brand new accounts have a "Photos" folder created for them, so that's a natural place to put them, but the Android app auto uploads photos to the "InstantUpload" folder by default. |
But when there is no photos directory select manually the app should function as it does with the default settings, I would assume. So this would also some your issue @strugee. Am I correct @Mikescops? :) |
By default no difference with the current version, but you can select one folder from where images on your timeline will be fetched from. |
? `<d:eq> | ||
<d:prop> | ||
<oc:owner-id/> | ||
</d:prop> | ||
<d:literal>${getCurrentUser().uid}</d:literal> | ||
</d:eq>` | ||
: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to not have the shares as results.
Why did you need to put a condition on it?
It should not cause any issues like it was before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The owner filter is only allowed on the root. Dav throws this error if you try on subfolders. What do you think then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it should be fixed on server.
cc @rullzer and @icewind1991 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->initialStateService->provideInitialState($this->appName, 'croppedLayout', $this->config->getUserValue($user->getUid(), Application::APP_ID, 'croppedLayout', 'false')); | ||
$this->initialStateService->provideInitialState($this->appName, 'timelineRootFolder', $this->config->getUserValue($user->getUid(), Application::APP_ID, 'timelineRootFolder', '/')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably limit the allowed keys here:
photos/lib/Controller/ApiController.php
Line 66 in 441f970
$user = $this->userSession->getUser(); |
As a safety mesure 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setUserConfig allows anything, and will put it straight into the db.
Meaning we could override existing values. We should check if the $key is either timelineRootFolder
or croppedLayout
and throw otherwise as a safety measure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok gotcha
@Mikescops right, to clarify, I'm not suggesting this would break existing workflows - I'm just saying that the feature might not actually be complete enough for many(?) users due to defaults in the server/Android app. Speaking only for myself, for example, I would not be able to use this feature since given the way my Nextcloud is organized, if I turned it on I would end up missing a ton of images that I'd want to see in the Photos app. @jancborchardt I'd be interested in hearing if you agree or if you think it's not worth the additional interface/code complexity. |
It's outside of the scope of this PR, if you're not happy with it, open a separate issue after it's merged, thanks. |
Which release will this go into? I'm looking forward to being able to use photos again, thank you @Mikescops |
NC 21 at least, for now we're stuck with a server issue. |
Hi! Sorry to bring this back, but I am hopeful this merge will resolve my issue. Is there any status update on this? |
Hey! The server hasn't received anything regarding this fix yet. Unfortunately this is not yet possible :/ |
Ah well that sucks, I was referring to this one: nextcloud/server#17941 mentioned some posts up. But would you happen to know what still needs to be done/added/fixed to make this work? I have a limited background in web development languages so I might be able to pull something through, even though I highly doubt it. |
It require a fair amount of rework on the backend dav search implementation :/ |
Sorry for being overly asking questions or coming off as pushy in any regard, I am merely just trying to figure out a good entry point, I looked through the code twice but I can't find an obvious stopping point beside the mentioned error with single vs multiple mime types being used. And I am unsure if you even know enough of this PR yourself to respond (It isn't easy going off using other people's work as a starting point for yourself, imo) but I still have no idea what is actually wrong with this code, what needs fixing and what doesn't. To me it looks mostly fine, given all it really needs to do is enter/search a single folder and mark that as the 'Photos' folder for NextCloud, if I get this correct. If I have more information I may try to play around with this to see if I can bring it to a working state somehow, I have a bad taste for using OneDrive as my image backup server ever since Samsung forced it upon me, and this still adds in 600 images from my several Unity projects. |
@StarSmasher44 I suggest you read the original issues like nextcloud/server#17941 #141 entirely (with the folded comments too) Again, this requires a fix on server. Currently there is a hard limitation for our webdav SEARCH that forces you to limit the search to a user home for performance issues.
Currently, the combo |
Closes #141